-
Notifications
You must be signed in to change notification settings - Fork 16.2k
Fix: Make sqlalchemy optional in Drill provider #59916
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Handle ImportError and raise AirflowOptionalProviderFeatureException when a method needs sqlalchemy.
|
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide (https://github.com/apache/airflow/blob/main/contributing-docs/README.rst)
|
sunank200
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also mark optional dependency for sqlalchemy in pyproject for drill
| conn_md = self.get_connection(self.get_conn_id()) | ||
|
|
||
| if create_engine is None: | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
|
||
| if create_engine is None: | ||
|
|
||
| raise AirflowOptionalProviderFeatureException("The 'sqlalchemy' library is required to use this hook.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| raise AirflowOptionalProviderFeatureException("The 'sqlalchemy' library is required to use this hook.") | |
| raise AirflowOptionalProviderFeatureException( | |
| "sqlalchemy is required for SQL filter clause generation. " | |
| "Install it with: pip install 'apache-airflow-providers-drill[sqlalchemy]'" | |
| ) |
|
I am actually not sure whether we need this PR and check at-all. Drill-provider has dependency to |
Agree. Looks like drill entirely depdens on sqlalchemy-drill -> it creates sqlalchemy engine in get_conn -> it does not use pure dbapi, just sqlalchemy to run queries. We can close it and mark as done. |
I modified the Drill provider to safely handle the absence of
sqlalchemyby wrapping the import in a try-except block. It now raisesAirflowOptionalProviderFeatureExceptioninget_connif the library is missing when actually needed.Testing:
I verified this manually by running a script that simulates a missing
sqlalchemyenvironment. This confirmed that while the parentDbApiHooktypically relies onsqlalchemy, the Drill provider now loads successfully without it and only raises the correct exception whenget_connis called.I look forward to your feedback.
closes: #59897